-
Notifications
You must be signed in to change notification settings - Fork 454
Rename lockdirs to discourage checking them into version control #12648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
428f1a2 to
934a26a
Compare
Dune's lock directories are currently not portable. Naming them "dune.lock" might suggest to users of other packgae managers that it is safe to check them into version control and share them between developers potentially using different machines. To discourage this behaviour, lockdirs are renamed from "dune.lock" to ".dune-solution-cache". The fact that this directory begins with a period and contains the word "cache" should hopefully discourage users from checking them into version control. For the same reason, the collection of dev tool lockdirs is renamed from "dev-tools.locks" to ".dune-tools-solution-cache". In the future it's likely we will enable portable lock directories by default, at which point they will become safe to check into version control. At this point we can change the default lock directory name back to "dune.lock" and encourage checking them into version control. Signed-off-by: Stephen Sherratt <[email protected]>
934a26a to
446d52a
Compare
|
This currently doesn't work because dune ignores directories beginning with a period. It works if you add |
|
Aren't we going to encourage users to check in the portable lock dirs? Morever, we already have a workflow where the lock directory doesn't even need to exist in the source directory at all and can be generated on the fly. Isn't that enough for everybody who doesn't want to commit these? |
Would it be feasible to just extend
#11775 has stalled because of issues with the current way dev-tools are implemented and it brings a lot of changes that haven't been tested. So if we want to publish the MSP reasonably soon it might need to descoped from the MSP. I would really like to see it succeed but if we need to re-implement dev-tools before landing #11775 that might just take too long. In any case, renaming |
|
I don't think renaming this directory is blocking your MSP anyhow. The lock directory is not just an implementation detail and should be committed when appropriate. Otherwise, why do we have portable lock directories at all? |
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of changes are unfortunately rather verbose but necessary. I've added some comments on where we can avoid changes and make the text a bit less specific.
| val debug_package_logs : bool ref | ||
|
|
||
| (** Whether we are ignoring "dune.lock/". *) | ||
| (** Whether we are ignoring ".dune-solution-cache/". *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update this comment to not mention the value. Especially since it doesn't just ignore dune.lock, it ignores every lock directory.
| @@ -1,4 +1,4 @@ | |||
| We test how opam files with depopts fields are translated into dune.lock files: | |||
| We test how opam files with depopts fields are translated into .dune-solution-cache files: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit misleading, as dune.lock is not a file.
| We test how opam files with depopts fields are translated into .dune-solution-cache files: | |
| We test how opam files with depopts fields are translated into lock directories: |
| Solution for .dune-solution-cache: | ||
| - foo.1.2 | ||
|
|
||
| $ find ${default_lock_dir} | sort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe adding something like | sed "s/^${default_lock_dir}/LOCK_DIR/" could be nice to avoid changing output could be a good idea here.
| @@ -1,4 +1,4 @@ | |||
| Test that shows what happens when dune.lock is ignored. | |||
| Test that shows what happens when .dune-solution-cache is ignored. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Test that shows what happens when .dune-solution-cache is ignored. | |
| Test that shows what happens when the lock dir is ignored. |
| > EOF | ||
|
|
||
| Building test works when the dune.lock is visible to dune. | ||
| Building test works when the .dune-solution-cache is visible to dune. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Building test works when the .dune-solution-cache is visible to dune. | |
| Building test works when the lock dir is visible to dune. |
| Specific lock files can be given as positional arguments. | ||
| $ outdated dune.lock | ||
| dune.lock is up to date. | ||
| $ outdated .dune-solution-cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use ${default_lock_dir} here?
|
|
||
| Local file system | ||
| $ runtest "(copy \"$PWD/dummy\")" 2>&1 | sed "s#$(pwd)#PWD#" | sed '/ *^\^*$/d' | sed '\#^File ".*dune.lock/foo.pkg", line 2, characters#d' | ||
| $ runtest "(copy \"$PWD/dummy\")" 2>&1 | sed "s#$(pwd)#PWD#" | sed '/ *^\^*$/d' | sed '\#^File ".*.dune-solution-cache/foo.pkg", line 2, characters#d' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use ${default_lock_dir} here?
| $ solve_project() { | ||
| > cat >dune-project | ||
| > dune pkg lock dune.lock | ||
| > dune pkg lock .dune-solution-cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| > dune pkg lock .dune-solution-cache | |
| > dune pkg lock "${default_lock_dir}" |
| > (lang dune 3.8) | ||
| > (lock_dir | ||
| > (path dune.lock) | ||
| > (path .dune-solution-cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this might work:
| > (path .dune-solution-cache) | |
| > (path ${default_lock_dir}) |
| total_number_of_transitive | ||
| in | ||
| let lock_dir_path = Stdune.Path.of_string "dune.lock" in | ||
| let lock_dir_path = Stdune.Path.of_string ".dune-solution-cache" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be read from somewhere.
But from what I see this doesn't really need to be changed, does it? It's just not the standard lock dir but the code presumably works with any path?
rgrinberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I've outlined, this is not the way to go. It is merely a cosmetic change in the short term, and is certainly not the way to go in the long term.
In the workflow of #11775 the lock dir is basically an implementation detail, the user does not have to create it nor even be aware of it. #12394 allows people to override the automatic locking with an explicit lock dir (which then should be committed!), but I don't think that that is going to be a default workflow for most users. As for portable lock dirs, they're not enabled by default yet. There are also some open questions on what the right behavior should be. But generally they should probably be made the default and replace non-portable lock dirs. |
|
With #11775, this PR becomes completely irrelevant as nothing is created in the source in the first place. So it's more of an argument to avoid this change.
Between the two alternatives, it could certainly be argued that you're picking the better default. But the problem is that we don't have two alternatives yet. Once we have them, we could have these discussions.
We do need to decide what "by default" is going to mean here. What are some of these open questions? I know there are issues with the implementation, but it's still a strict improvement over what we have for those that need this feature. |
|
(It's a bit straying of the path of this PR, because I think we should rather have this discussion in a PR about enabling portable lock dirs by default)
I think a big one is about locking on the platforms that are not in the default set. Say I am on FreeBSD, portable lock dirs will not generate a solution for my system, thus I can't build at all which is quite poor UX. Non-portable lock files work in such cases at the moment, generating a single non-portable FreeBSD solution. However adding the current system by default to the solution isn't a panacea, as someone relocking the same repo on a different system will then accidentally remove the FreeBSD solution again. Thus @gridbugs suggested printing an error saying "please add |
|
I'd suggest we only lock for the current platform and allow users to opt in and lock for others if they wish. If a mac user comes a across a linux lock file, they won't be able to run it. At which point they should relock, this would then lock both platoforms from then on. |
|
That would require the lock step to read what platforms are locked already. I don't think that's a particularly good design, because that makes the new lock dir dependent on previous lock dirs (hence @gridbugs' suggestion of opting in new platforms solutions via |
|
Yep, closing in favour of #12653. |
Dune's lock directories are currently not portable. Naming them "dune.lock" might suggest to users of other packgae managers that it is safe to check them into version control and share them between developers potentially using different machines. To discourage this behaviour, lockdirs are renamed from "dune.lock" to ".dune-solution-cache". The fact that this directory begins with a period and contains the word "cache" should hopefully discourage users from checking them into version control.
For the same reason, the collection of dev tool lockdirs is renamed from "dev-tools.locks" to ".dune-tools-solution-cache".
In the future it's likely we will enable portable lock directories by default, at which point they will become safe to check into version control. At this point we can change the default lock directory name back to "dune.lock" and encourage checking them into version control.